Skip to content

Fix Foldable1 (NonEmpty f) instance #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix Foldable1 (NonEmpty f) instance #39

wants to merge 2 commits into from

Conversation

rhendric
Copy link
Member

To be merged after #38 and purescript/purescript-tuples#39.

JordanMartinez
JordanMartinez reviewed Oct 22, 2020
View reviewed changes
src/Data/NonEmpty.purs Outdated
import Data.Ord (class Ord1)
import Data.Semigroup.Foldable (class Foldable1, foldMap1)
import Data.Semigroup.Foldable (foldl1) as Exports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that you are exporting this so that the Data.NonEmpty still exports foldl1, but I wonder whether we should keep that since Foldable1 now includes that as one of its members.

JordanMartinez
JordanMartinez suggested changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@JordanMartinez JordanMartinez left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment on the foldr1 implementation. I think other maintainers should weigh in on whether foldr1 should still be exported by Data.NonEmpty.

@JordanMartinez
Copy link
Contributor

When we resolve things here, it should be merged into #38. Otherwise, that PR won't pass CI.

@hdgarrood
Copy link
Contributor

I'd prefer to leave the type of the foldl1 we are exporting as-is; I think it's weird to re-export more general versions of functions like this (see also purescript/purescript-arrays#173). If I specifically imported foldl1 from NonEmpty but then it turned out I was able to run that foldl1 on a Tuple as well, I'd find that weird. We could maybe replace its implementation with the Foldable1 version though, to avoid duplication.

@rhendric
Copy link
Member Author

@hdgarrood Okay, how's this?

@hdgarrood
Copy link
Contributor

Looks good to me!

@JordanMartinez JordanMartinez mentioned this pull request Oct 23, 2020
Merged
srghma added a commit to srghma/purescript-arrays that referenced this pull request Oct 24, 2020
@srghma
fix: Fix Foldable1 NonEmptyArray instance (requires purescript/pure…
3bfb316 
…script-nonempty#39)
srghma added a commit to srghma/purescript-arrays that referenced this pull request Oct 24, 2020
@srghma
fix: Fix Foldable1 NonEmptyArray instance (requires purescript/pure…
478fb5b 
…script-nonempty#39)
@hdgarrood
Copy link
Contributor

Merged in #38. Thanks!

@hdgarrood hdgarrood closed this Oct 24, 2020
@srghma srghma mentioned this pull request Oct 24, 2020
Merged
@rhendric rhendric deleted the rhendric/foldr1-and-foldl1 branch October 24, 2020 15:32
@JordanMartinez JordanMartinez mentioned this pull request Oct 24, 2020
Closed
paluh pushed a commit to paluh/purescript-arrays that referenced this pull request Oct 26, 2020
@srghma @paluh
fix: Fix Foldable1 NonEmptyArray instance (requires purescript/pure…
38b691e 
…script-nonempty#39)
JordanMartinez added a commit to purescript/purescript-arrays that referenced this pull request Nov 6, 2020
@paluh @kl0tl
@srghma @JordanMartinez
Update to v0.14.0-rc3 (#181)
88e724d 
* Update TAG to v0.14.0-rc3; dependencies to master; psa to v0.8.0

* Add roles declarations to allow safe coercions

* fix: Fix `Foldable1 NonEmptyArray` instance (requires purescript/purescript-nonempty#39)

* fix: Fix `Foldable1 NonEmptyArray` instance -> swap foldl1Impl and foldr1Impl

* Fix warning by removing 'kind' word in import

Co-authored-by: Cyril Sobierajewicz <[email protected]>
Co-authored-by: Serhii Khoma <[email protected]>
Co-authored-by: JordanMartinez <[email protected]>
@thomashoneyman thomashoneyman mentioned this pull request Jan 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JordanMartinez JordanMartinez JordanMartinez requested changes

@hdgarrood hdgarrood hdgarrood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rhendric @JordanMartinez @hdgarrood